ADR for backend data initialization #3734
Conversation
📝 WalkthroughWalkthroughAdds an ADR that moves initial data loading to the backend: HomeController will generate bootstrap HTML embedding immutable AppData and optional InitialInstanceData, switches to path-based routing, reduces frontend initial queries, and outlines rollout, security considerations, pagination updates, and acceptance criteria. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
adamhaeger
added
kind/other
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
adr/002-backend-data-initialization.md (5)
112-121: Use proper headings instead of bold for section titles (fix MD036).Convert the emphasized lines to markdown headings.
-**Positive** +### Positive @@ -**Negative / Trade-offs** +### Negative / Trade-offs
130-133: Fix ordered list formatting in rollout plan.Numbers are missing periods/spaces and bold is misapplied.
-1**Route migration:** enable path-based routing; ship redirects for legacy hash URLs; validate deep links. -2**Delete old providers/effects** and RuleHandler/RuleConfiguration. -3**Docs & templates:** update app templates and docs; provide migration guide for app owners. +1. Route migration: enable path-based routing; ship redirects for legacy hash URLs; validate deep links. +2. Delete old providers/effects and RuleHandler/RuleConfiguration. +3. Docs & templates: update app templates and docs; provide migration guide for app owners.
94-100: Avoid optional chaining in inline bootstrap and add null checks.Safer and more compatible DOM access; keeps behavior identical.
- <script> - window.AltinnAppData = JSON.parse(document.getElementById('__appdata').textContent); - </script> - <script> - window.AltinnInitialInstanceData = JSON.parse( - document.getElementById('__initialInstanceData')?.textContent || 'null', - ); - </script> + <script> + (function () { + var elApp = document.getElementById('__appdata'); + if (elApp && elApp.textContent) { + window.AltinnAppData = JSON.parse(elApp.textContent); + } + })(); + </script> + <script> + (function () { + var elInst = document.getElementById('__initialInstanceData'); + window.AltinnInitialInstanceData = elInst && elInst.textContent + ? JSON.parse(elInst.textContent) + : null; + })(); + </script>
78-86: Clarify bootstrap schema versioning.Add an explicit version to the embedded contract to enable coordinated rollouts.
- **Bootstrap contract** + - Include an explicit `schemaVersion` in both `AppData` and `InitialInstanceData` to support backward/forward compatibility during deploys. @@ <script id="__appdata" type="application/json" > - { ... AppData ... } + { "schemaVersion": "1", ... AppData ... } </script>
103-106: Routing ops: add server fallback and proxy-aware handling.Ensure SPA deep links and asset paths work behind reverse proxies/CDNs.
- Serve a single controller/action for all app routes with a catch-all and do not 404 on refresh.
- Honor app basePath when generating asset URLs.
- Return 404/403 statuses while still serving the app shell for client-side routing to show error pages, as appropriate.
- Verify redirects from legacy hash routes are relative and cacheable with short TTL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
adr/002-backend-data-initialization.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
adr/002-backend-data-initialization.md
112-112: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
118-118: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
| ## Security & Compliance | ||
|
|
||
| - Ensure embedded data contains only what the current principal may see. | ||
|
|
||
| ## Rollout Plan |
There was a problem hiding this comment.
Harden security: escaping, CSP, and caching directives for embedded JSON.
Embedding instance/app data in HTML has XSS and caching risks. Document the requirements here.
## Security & Compliance
@@
-- Ensure embedded data contains only what the current principal may see.
+- Ensure embedded data contains only what the current principal may see.
+- Escape sequences that could break out of the JSON/script context (e.g., `</script`), or use a safe JSON serializer that guarantees this.
+- Set a strict CSP with nonce/hashes; forbid `unsafe-inline`. Example: `script-src 'self' 'nonce-…'`.
+- Serve bootstrap responses with `Cache-Control: private, no-store` (at minimum for instance-specific payloads) and `Vary: Cookie, Authorization` to avoid proxy/CDN leaks.
+- Set `X-Content-Type-Options: nosniff`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Security & Compliance | |
| - Ensure embedded data contains only what the current principal may see. | |
| ## Rollout Plan | |
| ## Security & Compliance | |
| - Ensure embedded data contains only what the current principal may see. | |
| - Escape sequences that could break out of the JSON/script context (e.g., `</script`), or use a safe JSON serializer that guarantees this. | |
| - Set a strict CSP with nonce/hashes; forbid `unsafe-inline`. Example: `script-src 'self' 'nonce-…'`. | |
| - Serve bootstrap responses with `Cache-Control: private, no-store` (at minimum for instance-specific payloads) and `Vary: Cookie, Authorization` to avoid proxy/CDN leaks. | |
| - Set `X-Content-Type-Options: nosniff`. | |
| ## Rollout Plan |
🤖 Prompt for AI Agents
adr/002-backend-data-initialization.md around lines 124-128: add concrete
security and caching requirements for any embedded JSON—require server-side
filtering to include only fields allowed for the current principal, serialize
with JSON.stringify and HTML-escape characters that can break HTML (<, >, &, /)
or prefix the payload (e.g., )]}',\n) to prevent JSON hijacking; mandate CSP
rules (script-src with nonces/hashes and default-src restrictions) and
X-Content-Type-Options: nosniff; specify caching policy: user-specific pages
must use Cache-Control: private or no-store and set appropriate Vary headers,
whereas truly public data may be cacheable with long max-age and immutable; add
automated tests and documentation notes for developers on safe embedding,
sanitization steps, and example headers.
| - Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions. | ||
|
|
There was a problem hiding this comment.
Resolve inconsistency: removal now vs. future removal.
Decision says remove RuleHandler/RuleConfiguration “in this major”, but acceptance criteria says “mention will be removed in future versions”. Align the plan.
-- Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions.
+- Remove RuleHandler/RuleConfiguration in this major and provide migration notes and deprecation notice in release docs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions. | |
| - Remove RuleHandler/RuleConfiguration in this major and provide migration notes and deprecation notice in release docs. |
🤖 Prompt for AI Agents
In adr/002-backend-data-initialization.md around lines 153-154, the ADR
currently contradicts itself by stating RuleHandler/RuleConfiguration will be
removed "in this major" while the acceptance criteria says to "mention will be
removed in future versions"; update the document to make both statements
consistent: choose the intended timeline (remove in this major OR deprecate and
remove in a future version), then edit the decision and acceptance criteria so
they match that choice and reflect the exact migration/deprecation steps and
user-facing notices required.
| When backend detects an instance context, it embeds (or exposes via single endpoint) the static-at-boot instance data adjacent to `AppData`, so the client does not immediately refetch/invalidate. Further interactions use targeted APIs. ([GitHub][1]) | ||
| 5. **Remove RuleHandler/RuleConfiguration in this major.** | ||
| Do not migrate them to the new boot flow (handled by separate task). | ||
|
|
There was a problem hiding this comment.
Could we also make a point of measuring the baseline, so that we can also measure the impact of these changes? 100% agreed on the principles of this, but would be even better if we had some data to point to. If we feel like auomated testing is out of scope, we could just include some manual measurements as text here based on a couple of waterfalls that are problematic/that we expect to improve. Examples
- Opening instance deeplink
- Render PDF
- ^ with and without subforms?
Even if it were just screenshots embedded into the markdown here it would help to motivate decisions and clarify which parts of the waterfalls are most important (or even not covered by the current proposal).
In the future we should have tests as code as well that can be ran to verify changes in performance over time.
There was a problem hiding this comment.
Since you mention TTFB/TTI, could include those perhaps
| ``` | ||
|
|
||
| - **Routing:** | ||
| - Backend maps `/` `/instances/:id/*` `/tasks/:taskId` etc. to a single `HomeController` action that embeds the correct initial payload and serves static assets. |
There was a problem hiding this comment.
Is it actually possible to make this work with our current API routes? E.g. we have {org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid} as a route in InstancesController. Do we get a collision if we remove the hash? Or was the frontend route instance (singular)?
There was a problem hiding this comment.
Good catch! The frontend route is singular; instance.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
adr/002-backend-data-initialization.md (2)
209-218: Security requirements still incomplete—add escaping, CSP, caching directives.We still need explicit guidance on sanitizing embedded JSON (escape
</script,<,>,&,/or use a serializer that does), enforcing a nonce-based CSP (script-src 'self' 'nonce-…', nounsafe-inline), and settingCache-Control: private, no-storeplusVary: Cookie, Authorization(andX-Content-Type-Options: nosniff) for any instance-scoped payload. Without these, teams can easily ship an XSS or leak data through shared caches. Please document these requirements here.
238-239: Align RuleHandler/RuleConfiguration timeline with the decision.Line 30 commits to removing RuleHandler/RuleConfiguration “in this major”, yet the acceptance criteria only promise a future removal notice. Please pick one plan and update both sections accordingly (e.g., either “remove in this major; document migration” or “deprecate now, remove in next major”).
🧹 Nitpick comments (1)
adr/002-backend-data-initialization.md (1)
215-217: Fix rollout numbering for readability.The rollout list renders as “1Route migration …” because the period is missing. Please change to
1. **Route migration:**,2. **Delete…**, etc., so Markdown renders the ordered list correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
adr/002-backend-data-initialization.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
adr/002-backend-data-initialization.md
197-197: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
203-203: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
adr/002-backend-data-initialization.md (2)
210-214: Harden security: escaping, CSP, and caching for embedded JSONSpell out concrete requirements to prevent XSS/leaks when embedding JSON in HTML.
Apply:
## Security & Compliance @@ - Ensure embedded data contains only what the current principal may see. + - Serialize with a safe JSON serializer that escapes characters that can break out of HTML/script contexts, including </script, <, >, &, and U+2028/U+2029. + - Use script-src CSP with nonces/hashes; forbid unsafe-inline. Example: Content-Security-Policy: default-src 'self'; script-src 'self' 'nonce-…'; object-src 'none'; base-uri 'none'. + - Serve bootstrap responses for user/instance-specific payloads with Cache-Control: private, no-store and Vary: Cookie, Authorization to avoid proxy/CDN leaks. + - Set X-Content-Type-Options: nosniff. + - Prefer embedding only small payloads; keep large/PII-heavy data behind authenticated APIs.
30-31: Resolve inconsistency on RuleHandler/RuleConfiguration timelineDecision says “Remove … in this major” while acceptance says “mention will be removed in future versions”. Align the plan.
Option A (remove now):
-5. **Remove RuleHandler/RuleConfiguration in this major.** +5. **Remove RuleHandler/RuleConfiguration in this major.** Do not migrate them to the new boot flow (handled by separate task). @@ -- Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions. +- Remove RuleHandler/RuleConfiguration in this major and provide migration notes and deprecation notice in release docs.Option B (deprecate now, remove later): update both sections accordingly.
Also applies to: 239-240
🧹 Nitpick comments (3)
adr/002-backend-data-initialization.md (3)
198-198: Use headings instead of bold labels (“Positive” / “Negative”)Improves structure and satisfies MD036.
-**Positive** +### Positive @@ -**Negative / Trade-offs** +### Negative / Trade-offsBased on static analysis hints.
Also applies to: 204-204
183-195: Deduplicate “Consequences” sectionsThere are two Consequences blocks; keep one to avoid confusion.
Suggested approach: remove the earlier ad-hoc block (lines 183–195) and keep the structured section starting at “## Consequences” (lines 196+).
-Consequences - -Positive: -... -Trade-offs: -...Also applies to: 196-209
233-239: Add measurable baseline/targets for perf (TTFB/TTI) and key flowsCapture current vs. expected improvements and how to measure them.
Add a short “Measurements” note here:
- Define scenarios: opening instance deep link, render PDF, with/without subforms.
- Record baseline TTFB/TTI and request counts; include screenshots/waterfalls.
- Set target thresholds (e.g., ≤2 requests pre-interactivity, TTFB ≤ X ms on P95).
- Plan to automate later with performance tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
adr/002-backend-data-initialization.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
adr/002-backend-data-initialization.md
198-198: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
204-204: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
adr/002-backend-data-initialization.md (2)
80-101: Bootstrap contract looks solidUsing script type application/json and JSON.parse via textContent is a safe, clear pattern. LGTM.
104-106: Confirm and align instance route naming
The ADR currently specifies/instances/:id/*; noInstancesControlleror related backend code was found. If the frontend uses singularinstance, update the ADR to/instance/:id/*and ensure no API route collisions.
| 1**Route migration:** enable path-based routing; ship redirects for legacy hash URLs; validate deep links. | ||
| 2**Delete old providers/effects** and RuleHandler/RuleConfiguration. | ||
| 3**Docs & templates:** update app templates and docs; provide migration guide for app owners. | ||
|
|
There was a problem hiding this comment.
Fix numbered list formatting in Rollout Plan
Malformed numbering (“1**Route…”) will render poorly.
-1**Route migration:** enable path-based routing; ship redirects for legacy hash URLs; validate deep links.
-2**Delete old providers/effects** and RuleHandler/RuleConfiguration.
-3**Docs & templates:** update app templates and docs; provide migration guide for app owners.
+1. Route migration: enable path-based routing; ship redirects for legacy hash URLs; validate deep links.
+2. Delete old providers/effects and RuleHandler/RuleConfiguration.
+3. Docs & templates: update app templates and docs; provide migration guide for app owners.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1**Route migration:** enable path-based routing; ship redirects for legacy hash URLs; validate deep links. | |
| 2**Delete old providers/effects** and RuleHandler/RuleConfiguration. | |
| 3**Docs & templates:** update app templates and docs; provide migration guide for app owners. | |
| 1. Route migration: enable path-based routing; ship redirects for legacy hash URLs; validate deep links. | |
| 2. Delete old providers/effects and RuleHandler/RuleConfiguration. | |
| 3. Docs & templates: update app templates and docs; provide migration guide for app owners. |
🤖 Prompt for AI Agents
In adr/002-backend-data-initialization.md around lines 216 to 219, the Rollout
Plan section contains malformed numbering ("1**Route migration:...") which will
render incorrectly; fix it by converting the lines to a proper Markdown numbered
list (e.g., "1. Route migration: ...", "2. Delete old providers/effects and
RuleHandler/RuleConfiguration.", "3. Docs & templates: ...") ensuring consistent
use of "1."/"2."/"3." and a space after the period so the list renders
correctly.
|
@adamhaeger I think we should move this PR to Altinn/altinn-studio, as ADRs have been places in a different folder there. |
github-project-automation
bot
moved this from ⚠️ Blocked
to ✅ Done
in Team Altinn Studio
3 participants
Description
The Frontend Next task force is proposing to load initial app data on the backend.
Related Issue(s)
Summary by CodeRabbit